Update llzk-pod-to-scalar to split pods within arrays to multiple arrays#565
Update llzk-pod-to-scalar to split pods within arrays to multiple arrays#565tim-hoffman wants to merge 121 commits into
llzk-pod-to-scalar to split pods within arrays to multiple arrays#565Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 477c5322c0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45632675f9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
7b9bf28 to
7da8b54
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8722c1ed4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
0xddom
left a comment
There was a problem hiding this comment.
The PodToScalarPass.cpp file is helper functions galore. The code is hard to follow and I think it could be organized a bit better.
| // CHECK-SAME: %[[ARR_X:[0-9a-zA-Z_\.]+]]: !array.type<2 x index>, | ||
| // CHECK-SAME: %[[ARR_Y:[0-9a-zA-Z_\.]+]]: !array.type<2 x !felt.type>, | ||
| // CHECK-SAME: %[[ELEM:[0-9a-zA-Z_\.]+]]: !poly.tvar<@E>) attributes {function.allow_constraint} { | ||
| // CHECK-NEXT: %[[ELEM_LEAVES:[0-9a-zA-Z_\.]+]]:2 = builtin.unrealized_conversion_cast %[[ELEM]] : !poly.tvar<@E> to index, !felt.type |
There was a problem hiding this comment.
I'm not sure if this cast can be easily reconciled without special handling since it goes 1:2 in terms of values. What happens if we feed this IR to the flattening pass?
There was a problem hiding this comment.
Oh, it seems I missed several unrealized_conversion_cast that were left behind and should be handled (if possible).
There was a problem hiding this comment.
It's fine to leave some unrealized conversions if passes down the line can clear them up
| // CHECK-NEXT: %[[VAL_4:[0-9a-zA-Z_\.]+]] = poly.unifiable_cast %[[VAL_0]] : (!array.type<2,? x !poly.tvar<@T>>) -> !array.type<2,? x index> | ||
| // CHECK-NEXT: %[[VAL_5:[0-9a-zA-Z_\.]+]] = poly.unifiable_cast %[[VAL_0]] : (!array.type<2,? x !poly.tvar<@T>>) -> !array.type<2,? x !felt.type> | ||
| // CHECK-NEXT: %[[VAL_6:[0-9a-zA-Z_\.]+]] = poly.unifiable_cast %[[VAL_0]] : (!array.type<2,? x !poly.tvar<@T>>) -> !array.type<2,? x none> |
There was a problem hiding this comment.
This cast feel wrong when compared to the others
There was a problem hiding this comment.
Yep. I thought I fixed that.
| // CHECK-LABEL: module attributes {llzk.lang} { | ||
| // CHECK-NEXT: function.def @named_array_result | ||
| // CHECK-SAME: (%[[VAL_0:[0-9a-zA-Z_\.]+]]: !array.type<2 x index>, %[[VAL_1:[0-9a-zA-Z_\.]+]]: !array.type<2 x !felt.type>) | ||
| // CHECK-SAME: -> (!array.type<2 x index> {function.res_name = "out.lhs"}, !array.type<2 x !felt.type> {function.res_name = "out.rhs"}) { |
There was a problem hiding this comment.
This may cause issues in llzk-spec since out.lhs is not a valid identifier. I'm not sure what the best solution would be. @iangneal thoughts?
| SplitPodArrayNonDetOp, SplitPodArrayCreateArrayOp, SplitPodArrayReadArrayOp, | ||
| SplitPodArrayWriteArrayOp, SplitPodArrayExtractArrayOp, SplitPodArrayInsertArrayOp, | ||
| SplitPodArrayInFuncDefOp, SplitPodArrayInReturnOp, SplitPodArrayInCallOp, | ||
| SplitPodArrayInEmitEqualityOp, SplitPodArrayInEmitContainmentOp, SplitPodArrayLengthOp>( |
There was a problem hiding this comment.
I think instead of handling those we should lower them into loops with another pass that should be called before this one if necessary.
| // CHECK-SAME: %[[ARR_X:[0-9a-zA-Z_\.]+]]: !array.type<2 x index>, | ||
| // CHECK-SAME: %[[ARR_Y:[0-9a-zA-Z_\.]+]]: !array.type<2 x !felt.type>, | ||
| // CHECK-SAME: %[[ELEM:[0-9a-zA-Z_\.]+]]: !poly.tvar<@E>) attributes {function.allow_constraint} { | ||
| // CHECK-NEXT: %[[ELEM_LEAVES:[0-9a-zA-Z_\.]+]]:2 = builtin.unrealized_conversion_cast %[[ELEM]] : !poly.tvar<@E> to index, !felt.type |
There was a problem hiding this comment.
It's fine to leave some unrealized conversions if passes down the line can clear them up
No description provided.